fix: accept legacy ACP optionIds for backward compatibility by RTCartist · Pull Request #436 · MoonshotAI/kimi-code

RTCartist · 2026-06-04T15:40:31Z

Related Issue

Resolve #426

Problem

Older ACP clients (Python ACP SDK 0.8.x) send approve / approve_for_session / reject as optionId values. The current TypeScript adapter only accepts the renamed canonical ids (approve_once / approve_always / reject), so legacy clients fall through permissionResponseToApprovalResponse's default branch and get silently rejected.

What changed

Add a LEGACY_OPTION_ID_MAP that maps legacy ids to their canonical equivalents and normalize before the decision switch in permissionResponseToApprovalResponse (only on the non-plan_review path — addresses a codex review concern that the plan mapper would otherwise see a normalized id). Also normalize in attachSelectedLabel so legacy approvals keep their human-readable selectedLabel (second codex review concern — without this, PermissionResult hooks and approval records would lose context like "Approve for this session").

Checklist

I have read the CONTRIBUTING document.
I have linked a related issue, or explained the problem above.
I have added tests that prove my feature works.
Ran gen-changesets skill, or this PR needs no changeset.
Ran gen-docs skill, or this PR needs no doc update.

Verification

pnpm -C packages/acp-adapter exec vitest run

Older ACP clients (e.g. Python ACP SDK 0.8.x) send different optionId values than the current TypeScript SDK: `approve` instead of `approve_once`, and `approve_for_session` instead of `approve_always`. Add a compatibility mapping in `permissionResponseToApprovalResponse` so these legacy ids are normalized to their canonical equivalents before the decision switch, preventing valid approvals from being silently rejected. Closes MoonshotAI#426

changeset-bot · 2026-06-04T15:40:37Z

🦋 Changeset detected

Latest commit: 416f9b7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@moonshot-ai/acp-adapter	Patch
@moonshot-ai/kimi-code	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1955dd9da2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-06-04T15:44:23Z

+  const optionId = LEGACY_OPTION_ID_MAP[rawOptionId] ?? rawOptionId;
  if (req?.display.kind === 'plan_review') {
    return mapPlanReviewOptionId(req.display, optionId);


Handle legacy approval in plan-review fallback

When an older ACP client answers a plan-review fallback prompt with the legacy approve id, this normalizes it to approve_once and then immediately routes it through the plan-review mapper, which only accepts plan_approve/plan_* ids and returns { decision: 'rejected' }. This affects the real handleApproval path because it always passes the original req; for display.kind === 'plan_review' with no multi-option variants, the single Approve button is still rejected for the legacy clients this change is meant to support.

Useful? React with 👍 / 👎.

Move the legacy-to-canonical normalization below the plan_review check so that plan_review prompts pass the raw optionId directly to mapPlanReviewOptionId. Legacy clients predate plan_review, but this ordering is defensively correct: a stray canonical id should not be silently rewritten before the plan mapper sees it.

RTCartist · 2026-06-04T16:24:16Z

Good catch — moved the legacy normalization below the plan_review check so mapPlanReviewOptionId always receives the raw optionId. Legacy clients predate plan_review so this is a theoretical concern, but the new ordering is defensively correct. Fixed in 9bba513.

RTCartist · 2026-06-04T16:38:15Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9bba5139ea

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-06-04T16:42:01Z

@@ -0,0 +1,5 @@
+---
+"@moonshot-ai/kimi-code": patch


Include the changed ACP adapter in the changeset

The repo's changeset rules require source changes that affect a package's output to list that package, and to additionally list the CLI when an internal package change enters the CLI bundle. This commit changes packages/acp-adapter/src/approval.ts, but the changeset only bumps @moonshot-ai/kimi-code, so @moonshot-ai/acp-adapter will not be versioned/tracked for this fix; add "@moonshot-ai/acp-adapter": patch here as well.

Useful? React with 👍 / 👎.

… fix Source changes live in `packages/acp-adapter/src/approval.ts`, so the adapter package needs to bump alongside `@moonshot-ai/kimi-code` to match the repo convention used by prior internal-package changes (see e.g. .changeset/add-update-config-skill.md).

RTCartist · 2026-06-04T18:04:55Z

Good catch — confirmed against prior merged changesets in this repo. For example, .changeset/add-update-config-skill.md (PR #433) lists both @moonshot-ai/agent-core and @moonshot-ai/kimi-code when source changes touch packages/agent-core/. Added @moonshot-ai/acp-adapter: patch to this changeset to match the convention (8eb1847). Also applied the same fix to #438 which had the same issue.

RTCartist · 2026-06-04T18:28:06Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8eb18476af

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-06-04T18:33:13Z

-    return mapPlanReviewOptionId(req.display, optionId);
+    return mapPlanReviewOptionId(req.display, rawOptionId);
  }
+  const optionId = LEGACY_OPTION_ID_MAP[rawOptionId] ?? rawOptionId;


Preserve labels for legacy approvals

When a legacy client returns approve or approve_for_session, this normalization only affects the decision mapper; AcpSession.handleApproval still passes the original response into attachSelectedLabel, whose lookup compares the raw legacy id against the canonical options table and returns without adding a label. In the legacy-client path fixed here, approvals now execute but PermissionResult hooks/approval records lose the expected selectedLabel such as "Approve for this session", unlike the canonical path. Consider reusing the normalized id when attaching the label or teaching attachSelectedLabel the same legacy aliases.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-06-04T18:33:13Z

@@ -0,0 +1,6 @@
+---
+"@moonshot-ai/acp-adapter": patch


Drop the private package from the changeset

This changeset selects @moonshot-ai/acp-adapter, but that workspace is marked private in packages/acp-adapter/package.json and the repository release guidance says not to add release changesets for private internal packages, only the publishable surface packages. Leaving this entry causes the release PR to version/report an internal adapter package instead of only bumping the user-visible package affected by the fix.

Useful? React with 👍 / 👎.

When a legacy client (Python ACP SDK 0.8.x) returns `approve` or `approve_for_session`, `permissionResponseToApprovalResponse` correctly routes the decision but `attachSelectedLabel` then looks up the raw legacy id against the canonical option table and misses, dropping the human-readable label. PermissionResult hooks and approval records would therefore lose context such as "Approve for this session" — unlike the canonical path. Reuse `LEGACY_OPTION_ID_MAP` in `attachSelectedLabel` to normalize the optionId before the lookup, matching the canonical path.

RTCartist · 2026-06-04T18:40:42Z

Two pieces of feedback in this round — addressed the label issue, deferring on the changeset:

1. Label preservation (P3) — real bug, fixed in 416f9b7.

Reused LEGACY_OPTION_ID_MAP inside attachSelectedLabel so the lookup against CANONICAL_OPTIONS normalizes the legacy id first. Without this, legacy approve_for_session → approve_always was correctly routed by the decision mapper but attachSelectedLabel then missed the canonical-table lookup (table key is approve_always, not the raw approve_for_session), so selectedLabel was dropped. Added 2 tests covering approve and approve_for_session → "Approve once" / "Approve for this session". All 244 acp-adapter tests pass.

2. Changeset (P3) — this one conflicts with the bot's earlier review.

The previous Codex review on this same PR flagged the opposite — that I should add @moonshot-ai/acp-adapter because packages/acp-adapter/src/approval.ts is what changed. I added it citing precedent: e.g. .changeset/add-update-config-skill.md (PR #433) lists @moonshot-ai/agent-core (also private: true) alongside @moonshot-ai/kimi-code.

The current review reads the README literal text ("Do not add release changesets for private internal packages"), which contradicts that precedent. Happy to drop the entry if maintainers prefer the README rule — leaving it for now since the precedent appears active.

RTCartist · 2026-06-04T18:44:43Z

@codex review

chatgpt-codex-connector · 2026-06-04T18:46:53Z

Codex Review: Didn't find any major issues. 👍

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

liruifengv · 2026-06-05T03:30:04Z

Sorry, we currently do not accept PRs submitted in batches using AI

RTCartist · 2026-06-05T03:31:31Z

Sorry, we currently do not accept PRs submitted in batches using AI

I think you at least need to review the pr and make decision. All prs have been checked and reviewed by myself.

chore: add changeset for legacy ACP optionId compatibility fix

0bbf492

chatgpt-codex-connector Bot reviewed Jun 4, 2026

View reviewed changes

liruifengv closed this Jun 5, 2026

Conversation

RTCartist commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issue

Problem

What changed

Checklist

Verification

Uh oh!

changeset-bot Bot commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

RTCartist commented Jun 4, 2026

Uh oh!

RTCartist commented Jun 4, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

RTCartist commented Jun 4, 2026

Uh oh!

RTCartist commented Jun 4, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

RTCartist commented Jun 4, 2026

Uh oh!

RTCartist commented Jun 4, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jun 4, 2026

Uh oh!

liruifengv commented Jun 5, 2026

Uh oh!

RTCartist commented Jun 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

RTCartist commented Jun 4, 2026 •

edited

Loading

changeset-bot Bot commented Jun 4, 2026 •

edited

Loading